Allow subclasses to use _fromObject. #5917

Harmageddon · 2019-10-10T15:54:04Z

The default fromObject methods for fabric objects use fabric.Object._fromObject. This works for subclasses, too, if you define the fromObject method like this:

fabric.CustomRect.fromObject = function (object, callback) {
	return fabric.Object._fromObject('CustomRect', object, callback);
};

However, this works only for subclasses within the fabric "namespace". For other namespaces, _fromObject fails to resolve the correct constructor, which means you would have to write your own fromObject method if you have subclasses in other namespaces.

Demo

See the demo: http://jsfiddle.net/5xaq1rz6/5/

Here, we have three kinds of rectangles. The blue one is a normal fabric.Rect, while the other two are instances of subclasses. To make the difference visible, the subclasses only allow their instances to be moved horizontally.
The red rectangle is an instance of fabric.CustomRect, so within the fabric namespace. Clone it by selecting it and clicking the clone button, and you'll see that a new instance of the same subclass is generated correctly.
The green rectangle is an instance of Custom.Rect, a subclass in a different namespace. Clone it, and you'll see that the new instance is a normal rectangle instead of the subclassed one, because the definition for fromObject holds:

Custom.Rect.fromObject = function (object, callback) {
	return fabric.Object._fromObject('Rect', object, callback);
};

So the constructor used for the new object will be fabric.Rect.

Proposed changes

With this change, instead of strings defining the class names, classes themselves can be passed as a className parameter. The code for our rectangle subclass would now be:

Custom.Rect.fromObject = function (object, callback) {
	return fabric.Object._fromObject(Custom.Rect, object, callback);
};

and the object can be reconstructed correctly. For all classes inside the fabric namespace, nothing changes in the usage of fromObject.

asturur · 2019-10-27T17:25:11Z

This change would be good for me, but how would we solve full canvas restoration?
In that case classes have still to be appended to fabric in order to be restored.

we have still this utility here that is a big part of the process

    /**
     * Returns klass "Class" object of given namespace
     * @memberOf fabric.util
     * @param {String} type Type of object (eg. 'circle')
     * @param {String} namespace Namespace to get klass "Class" object from
     * @return {Object} klass "Class"
     */
    getKlass: function(type, namespace) {
      // capitalize first letter only
      type = fabric.util.string.camelize(type.charAt(0).toUpperCase() + type.slice(1));
      return fabric.util.resolveNamespace(namespace)[type];
    },

How do you suggest we come back from a full string representation?

asturur · 2019-10-27T17:28:11Z

maybe with the use of namespace and adding a namespace property in objects exports it can work, assuming that in 2020 using window and global is still fine for developers.

Let me know if you want to push this forward and if you need some help.

stale · 2020-01-25T22:36:43Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Harmageddon · 2020-01-26T16:07:20Z

How do you suggest we come back from a full string representation?

Good point! I extended the JSFiddle to contain import/export: https://jsfiddle.net/Harmageddon/qwbo6053/
As you suggested, I introduced a "namespace" property and used getKlass in fabric.Object._fromObject to make use of the namespace. But now the toObject tests fail, probably because namespace is not in the reference objects. Do you prefer to add this to the tests or to only set the namespace attribute in toObject when different from null/undefined?

asturur · 2020-02-02T12:30:12Z

i ll get back to this, i m currently a bit busy

Harmageddon · 2020-02-02T13:35:03Z

Ok, no problem!

thorstenv · 2020-02-05T13:50:49Z

i would be very interested in this feature too. Using angular with fabricjs, i dont seem to be able to create custom compontents that can be deserialized (fabric cant find them).

asturur · 2020-02-06T07:43:44Z

you can deserialize back if you add them to fabric namespace following the lowerCase schema.

if the class is CustomRect the type has to be set to 'customRect'

asturur · 2020-02-16T16:13:23Z

@Harmageddon i see you have changed approach a bit. I would be fine with this change if you can care to document with an example. Does not seem that the default is changed.

asturur · 2020-02-16T16:13:46Z

well broken UTs apart.

asturur · 2020-02-16T16:15:32Z

What about making a rule? object.type can be something.somethingElse where something would be the namespace and somethingElse would be the class name. In this case if type contains a dot we would apply some rule.
Just an idea.

Harmageddon · 2020-02-16T16:54:31Z

@Harmageddon i see you have changed approach a bit. I would be fine with this change if you can care to document with an example. Does not seem that the default is changed.

Yes, as mentioned above, because it seems to be a cleaner solution, and because getKlass already supports a namespace argument.

well broken UTs apart.

That's because of the toObject method being changed, as I mentioned here:

But now the toObject tests fail, probably because namespace is not in the reference objects. Do you prefer to add this to the tests or to only set the namespace attribute in toObject when different from null/undefined?

So either we add the namespace property to the expected value in the unit tests (if we want the namespace property to be exported always), or we just don't export it in toObject when unset, and maybe add a unit test for custom namespaces.

What about making a rule? object.type can be something.somethingElse where something would be the namespace and somethingElse would be the class name. In this case if type contains a dot we would apply some rule.
Just an idea.

That's also possible, would just need to change getKlass then. But then what to do for getKlass('Custom.Rect', 'SomeOtherNamespace')?

For my use case, I provided an example with the fiddle at https://jsfiddle.net/Harmageddon/qwbo6053/, but would be happy to provide more documentation if wanted.

asturur · 2020-02-16T18:05:12Z

i think if we export the namespace only when is different from falsy and we make test pass, plus we add a test with a custom namespace, we are good to go.

Harmageddon · 2020-02-22T14:26:33Z

I modified the toObject to only include namespace if not falsy and added two tests: toObject with namespace and clone with namespace. The latter fails in the node environment, because window is not available. @asturur do you have an idea how this test has to look in order to work?

asturur · 2020-02-23T23:17:51Z

you can try to swap window with document.defaultView, that should help to pass

Harmageddon · 2020-02-27T16:27:08Z

Somehow I got Github to hiccup by altering the commit history here. I can create a new PR or maybe it can be solved when merging by squashing.
However, neither using document.defaultView nor global worked. When I create global.Custom, it works within the test, but inside resolveNamespace, Custom seems to be undefined.

asturur · 2020-02-27T17:06:38Z

merge + squash should do it.

asturur · 2020-02-27T17:08:02Z

so i see there is will to solve here. i do not want to have this stopped forever.
Can you please ensure i have write access to the branch on your repo? there is some option around enabling the author to modify the fork. I want to make a couple tries at fixing the test and merge it.

Harmageddon · 2020-02-27T17:38:06Z

Thank you! I have enabled "Allow edits from maintainers." for this PR.

asturur · 2020-02-28T10:43:52Z

The problem is that resolve namespace is wrapped in this statement were global is exports under node.

asturur · 2020-02-28T11:14:41Z

diff --git a/src/util/misc.js b/src/util/misc.js
index 969aff64..7856dcd0 100644
--- a/src/util/misc.js
+++ b/src/util/misc.js
@@ -1,4 +1,4 @@
-(function(global) {
+(function() {
 
   var sqrt = Math.sqrt,
       atan2 = Math.atan2,
@@ -318,7 +318,7 @@
 
       var parts = namespace.split('.'),
           len = parts.length, i,
-          obj = global || fabric.window;
+          obj = typeof global === 'undefined' ? fabric.window : global;
 
       for (i = 0; i < len; ++i) {
         obj = obj[parts[i]];
@@ -975,4 +975,4 @@
       }).join(' ') + ')';
     }
   };
-})(typeof exports !== 'undefined' ? exports : this);
+})();
diff --git a/test/unit/object.js b/test/unit/object.js
index dff18a80..c018aa58 100644
--- a/test/unit/object.js
+++ b/test/unit/object.js
@@ -1,4 +1,4 @@
-(function(){
+(function(global){
 
   var canvas = this.canvas = new fabric.StaticCanvas(null, {enableRetinaScaling: false});
 
@@ -464,21 +464,21 @@
   });
 
   QUnit.test('clone with namespace', function(assert) {
-    var glob = fabric.isLikelyNode ? global : window;
-
-    glob.Custom = {
-      Object: fabric.util.createClass(fabric.Object, {
-        'namespace': 'Custom',
-      })
+    var MyClass = fabric.util.createClass(fabric.Object, {
+      namespace: 'MyNameSpace',
+      type: 'myClass'
+    });
+    MyClass.fromObject = function(object, callback) {
+      return fabric.Object._fromObject('MyClass', object, callback);
     };
-
-    var cObj = new glob.Custom.Object();
-
+    global.MyNameSpace = {
+      MyClass: MyClass
+    };
+    var cObj = new MyClass();
     assert.ok(typeof cObj.clone === 'function');
     cObj.clone(function(clone) {
-      assert.ok(clone instanceof glob.Custom.Object);
-
-      delete glob.Custom;
+      assert.ok(clone instanceof MyClass);
+      delete global.MyNameSpace;
     });
   });
 
@@ -1289,4 +1289,4 @@
     object.fill = 'transparent';
     assert.equal(object.hasFill(), false, 'with a color that is transparent, hasFill is true');
   });
-})();
+})(typeof global !== 'undefined' ? global : this);

asturur · 2020-02-28T11:15:14Z

i can't push, unsure what is wrong, that is what i did, could you please merge it in your branch and let's move forward from there.

Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>

Harmageddon · 2020-02-28T16:56:34Z

Awesome, it works! Thank you!

ShaMan123 · 2022-01-22T20:32:04Z

This discussion should be revived coming into v6.
I think we will have to implement something in the direction suggested here.

ShaMan123 · 2022-01-22T20:36:30Z

I prefer some kind of global registry/map of classes that fabric holds.

fabric.registerClass(type /** id */, clazz)

and/or perhaps another option for overriding while loading from JSON?

and/or have a resolver that the dev can completely override:

fabric.resolveClass = function(type) {
//  return a class constructor
}

ShaMan123 · 2022-02-15T21:28:55Z

#7657 solves the broader issue and makes this PR stale
especially 9f5ef64

stale bot added the stale Issue marked as stale by the stale bot label Jan 25, 2020

Harmageddon force-pushed the clone_subclassed branch from c5a5e38 to 03c469f Compare January 26, 2020 15:44

stale bot removed the stale Issue marked as stale by the stale bot label Jan 26, 2020

asturur added the possible_feature label Feb 2, 2020

Harmageddon added 2 commits February 15, 2020 16:32

Allow subclasses to use _fromObject.

529e370

Introduced namespace attribute for objects.

7f7827e

Harmageddon force-pushed the clone_subclassed branch from 03c469f to 7f7827e Compare February 15, 2020 15:33

Harmageddon added 4 commits February 22, 2020 14:54

Only export namespace when not falsy.

7b2f849

Added toObject and clone test for custom namespace.

95ad8a1

Fixed clone with namespace test.

465cfb4

Fixed linting of clone with namespace test.

fcf84ed

Replaced window by document.defaultView in test.

f30c782

Harmageddon force-pushed the clone_subclassed branch 3 times, most recently from caf2696 to f30c782 Compare February 27, 2020 15:13

Use global instead of window when in node.

ba90cd3

Fixed clone with namespace test.

89cb643

Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>

Merge branch 'master' into clone_subclassed

489b3ae

ShaMan123 closed this Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow subclasses to use _fromObject. #5917

Allow subclasses to use _fromObject. #5917

Harmageddon commented Oct 10, 2019

asturur commented Oct 27, 2019

asturur commented Oct 27, 2019

stale bot commented Jan 25, 2020

Harmageddon commented Jan 26, 2020

asturur commented Feb 2, 2020

Harmageddon commented Feb 2, 2020

thorstenv commented Feb 5, 2020

asturur commented Feb 6, 2020

asturur commented Feb 16, 2020

asturur commented Feb 16, 2020

asturur commented Feb 16, 2020

Harmageddon commented Feb 16, 2020

asturur commented Feb 16, 2020

Harmageddon commented Feb 22, 2020

asturur commented Feb 23, 2020

Harmageddon commented Feb 27, 2020

asturur commented Feb 27, 2020

asturur commented Feb 27, 2020

Harmageddon commented Feb 27, 2020

asturur commented Feb 28, 2020

asturur commented Feb 28, 2020

asturur commented Feb 28, 2020

Harmageddon commented Feb 28, 2020

ShaMan123 commented Jan 22, 2022

ShaMan123 commented Jan 22, 2022 •

edited

Loading

ShaMan123 commented Feb 15, 2022 •

edited

Loading

Allow subclasses to use _fromObject. #5917

Allow subclasses to use _fromObject. #5917

Conversation

Harmageddon commented Oct 10, 2019

Demo

Proposed changes

asturur commented Oct 27, 2019

asturur commented Oct 27, 2019

stale bot commented Jan 25, 2020

Harmageddon commented Jan 26, 2020

asturur commented Feb 2, 2020

Harmageddon commented Feb 2, 2020

thorstenv commented Feb 5, 2020

asturur commented Feb 6, 2020

asturur commented Feb 16, 2020

asturur commented Feb 16, 2020

asturur commented Feb 16, 2020

Harmageddon commented Feb 16, 2020

asturur commented Feb 16, 2020

Harmageddon commented Feb 22, 2020

asturur commented Feb 23, 2020

Harmageddon commented Feb 27, 2020

asturur commented Feb 27, 2020

asturur commented Feb 27, 2020

Harmageddon commented Feb 27, 2020

asturur commented Feb 28, 2020

asturur commented Feb 28, 2020

asturur commented Feb 28, 2020

Harmageddon commented Feb 28, 2020

ShaMan123 commented Jan 22, 2022

ShaMan123 commented Jan 22, 2022 • edited Loading

ShaMan123 commented Feb 15, 2022 • edited Loading

ShaMan123 commented Jan 22, 2022 •

edited

Loading

ShaMan123 commented Feb 15, 2022 •

edited

Loading